Codecov Report

Merging #27 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #27   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines         116    134   +18     
  Branches       12     13    +1     
=====================================
+ Hits          116    134   +18

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f44ac6c...074657e. Read the comment docs.

Thanks for the PR. Have you taken a look at my proposal #2 for throttle? It seems very similar, except that inmediate (should be immediate btw) is always true. Do you have use cases where you always want the first time an action is called to be delayed? I would prefer delay over wait if we're going with debounce, but my real preference would be more of an action rate limiter/throttle with a prop of rate.

You're welcome. I had a look at #2 after I after I created this PR. I've prepared a throttle implementation, but I've not prepared the test yet.

throttleand debounce seem very similar, but the behaviour is subtly different. I've always liked this demo to visualise the difference.

Use case: listening for typing in a search ahead box. I don't want the search to start immediately, I want to start the search when the user has finished typing.

@rmc thanks for the reference, I believe that confirms my beliefs WRT throttle vs debounce.

I hear folks mentioning the search box as a use case for debounce, but personally that user experience bugs me since you won't see the results update while you're typing. The ideal experience IMHO would be to trigger a search after each space immediately (more likely to match when you have a phrase with complete words) and throttle searches while the user is typing other characters so they don't overload your API.

Should we have both throttle and debounce effects? Or is there a way to write one effect that can do the work of both?

@okwolf I liked your suggestion for searching and was able to implement it with fxIf really easily (naive version):

    startSimpleSearch: event => fxIf([
      [/[^\s]$/.test(event.target.value), debounce(500, 'search')],  // last char is not whitespace
      [/\s$/.test(event.target.value), action('search')], // last char is whitespace
      [/^(?![\s\S])/.test(event.target.value), action('noop')] // had to add this for reasons I'll explain later
    ]

I'd go for adding both throttle and debounce. That way you could choose which you prefer according to your sensibilities/requirements. Writing one function to handle both cases would be complicated. Specifying the effect you would like to use explicitly makes more sense to me.

As for the last line in my example: I was implementing your search experience, and noticed that whenever all the conditions in fxIf evaluated to false, fxIf returns an empty array and the following error appears:

So I implemented a noop action, now whenever event.target.value is "" the error doesn't appear anymore. TBH I think I might be abusing fxIf. Am I right in thinking it always requires one condition to evaluate to true? I might be looking for and fxMaybe; execute all effects for which the condition evaluates to true? (This is a whole different issue though)

@rmc thanks for trying out my crazy harebrained scheme!

I'm OK with adding both throttle and debounce since neither should be very large functions. How attached are you to having the immediate prop on debounce are you? I would think this feature wouldn't be used most of the time, so I'm thinking we should leave it out until someone really needs it. I'm also not a big fan of boolean arguments in general, so if we do find that we need such a thing, we might want to migrate to passing objects directly for the props to make it more like named arguments, but that's a different discussion...

I believe you are using fxIf correctly, but the code for running fx has a bug where it should treat empty arrays as a no-op effect instead of trying to run them. Like you said, that should be a different PR though. And that PR is #28.

Not really attached to immediate, removing it should simplify debouncea bit.

Should I attach the throttle effect to this PR as well?

Good to know I wasn't abusing fxIf too much ;-).

@rmc Not really attached to immediate, removing it should simplify debouncea bit.

👍

@rmc Should I attach the throttle effect to this PR as well?

Your call, I'd like to get both in before making a new release either way. Also the bug fix for #28.

@rmc thanks for the simplification!

I think we just need docs and this effect will be 💯.

Docs are ready to go. Please have a look at them to make sure they're clear enough.